Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(credentials): implement form with matchExpression #481

Merged
merged 22 commits into from
Jul 14, 2022

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jul 11, 2022

Fixes #467

  • fix(credentials): use new API v2.2 credentials format
  • chore(credentials): extract to subdirectory
  • feat(jmx): remove cached credentials, use backend storage only
  • feat(auth): add link from auth modal to /security
  • feat(credentials): user can enter matchExpression for credentials
  • update test
  • fixup! feat(auth): add link from auth modal to /security

Test with https://github.com/cryostatio/cryostat/pull/1019 on the backend.

@andrewazores andrewazores added the feat New feature or request label Jul 11, 2022
Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm curious if you have seen the consensus on using React.FunctionComponent here facebook/create-react-app#8177 .

It seems that the React developers say that using React.FC is unnecessary because of the issues they detail. Wanted to get your thoughts on that.

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I'm wondering if there should also be the hint like in the Automated Rule create wizard? For me I didn't know what a match expression was supposed to look like so it may be helpful.

@andrewazores
Copy link
Member Author

In general, I'm curious if you have seen the consensus on using React.FunctionComponent here facebook/create-react-app#8177 .

It seems that the React developers say that using React.FC is unnecessary because of the issues they detail. Wanted to get your thoughts on that.

I haven't seen that before. I don't really have any thoughts on the matter or opinion one way or the other.

https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/function_components/

I tend to like the explicit/static types where possible, but sometimes it isn't really necessary - and if the type signature is actually lying to you, like it is about the implicit children, then it does more harm than good.

If you'd like to go through and refactor to remove all those type declarations later then I wouldn't be opposed.

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! And all tests passed for me :D

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the match expression hint thing, which is up to you of course, looks great.
Good work!

@andrewazores
Copy link
Member Author

Other than the match expression hint thing, which is up to you of course, looks great. Good work!

There is a hint now, but it's tucked away inside of a tooltip icon so that the modal contents aren't too large and the user doesn't have to scroll too much.

@maxcao13
Copy link
Member

maxcao13 commented Jul 13, 2022

I haven't seen that before. I don't really have any thoughts on the matter or opinion one way or the other.

https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/function_components/

I tend to like the explicit/static types where possible, but sometimes it isn't really necessary - and if the type signature is actually lying to you, like it is about the implicit children, then it does more harm than good.

If you'd like to go through and refactor to remove all those type declarations later then I wouldn't be opposed.

Maybe I will do a refactor in the future (the docs you linked has something for that anyways), but for now I think it may be best to stop using React.FC in -web in the future and stick with the usual

const Foo = ( {prop1, prop2}: FooProps ): JSX.Element => {
   ...
   return ...
}

so that everything is using the same and preferred style.

I'll open an issue for it, if that's okay with you anyways. This isn't a big deal by any means ^^. I'm not sure what is the best way to let everyone know though, haha.

@andrewazores
Copy link
Member Author

I'll open an issue for it, if that's okay with you anyways. This isn't a big deal by any means ^^. I'm not sure what is the best way to let everyone know though, haha.

The best way is probably just to remove it from the codebase. New code added will naturally tend to follow the existing style found as new developers onboard and use what they read as a reference for what they write. We could also add it to a style guide document, but if it's just a written suggestion and not enforced by formatting/linting tools then it's likely to just get ignored or missed.

@tthvo
Copy link
Member

tthvo commented Jul 14, 2022

@andrewazores I just have a quick question.

If I enter the credential form with the exact same inputs (i.e. match expression, username, password - I guess this could be duplicate) multiple times, the credentials are appended as new. Is this intentional?

@andrewazores
Copy link
Member Author

Not exactly intentional, but harder to guard against than it seems.

The username and password can definitely be duplicated across different definitions.

The matchExpression should be unique, conceptually, but that's not as easy as it sounds.

A || B
A    ||    B
A||B
!(!A && !B)
A || (A || B)

are all "the same expression", but we won't catch those by simply doing string1.equals(string2).

We could respond with a 400 and reject cases where the exact same matchExpression is entered a second time, where the String representation is identical, but that's only a tiny sliver of semantically duplicated matchExpressions that could be entered.

If we could catch all of those cases then for sure, I would want to reject semantically duplicated expressions. But right now we don't have any engine for parsing and evaluating the semantics of the expression to determine logical equivalence, so I leave it as permissive and allow the user to do silly things by duplicating expressions, rather than explicitly rejecting only certain cases of identical strings.

@tthvo
Copy link
Member

tthvo commented Jul 14, 2022

Ahh make sense! It is really hard to check those.

But right now we don't have any engine for parsing and evaluating the semantics of the expression to determine logical equivalence

Have we considered any engine yet?

@andrewazores
Copy link
Member Author

No, and to be honest I'm not even sure such a thing exists for this use case. This is a kind of formal verification, which is an advanced topic, and JavaScript (which is what our matchExpressions really are) seems particularly hard to formally verify because of all its dynamic behaviours and types. And if it does exist, we would need it to be available as a Java package we can import as a dependency, or at least as some C library we can call out to over JNI. That's a very tight set of requirements to find anything that fits, and if it does exist I'm not sure it's even worth the effort/cost.

@tthvo
Copy link
Member

tthvo commented Jul 14, 2022

Ahh got it! It sureee seems to introduce too much complication :D I think we are all good for merging?

@andrewazores
Copy link
Member Author

Oh... and I forgot about some more "easy" and "obvious" permutations too!

A || B
A    ||    B
A||B
!(!A && !B)
A || (A || B)

B || A
B || A || false
(A || B) && true

You can almost fake a solution by testing the new matchExpression against all known targets and creating a Set<ServiceRef> where the set contains all and only ServiceRefs where the expression evaluates truthy. Then, test every other already-known matchExpression in storage the same way. The new expression appears to be a duplicate of an existing expression if the Set<ServiceRef> .equals() each other. But if you have a lot of known targets and/or a lot of already defined matchExpressions this gets really costly. And even worse, it still isn't correct, because if for example I'm running in OpenShift and have labelled every Pod I have with mylabel=foo, then the expression target.labels.mylabel === \"foo\" and the expression true appear to be semantically equivalent!

So the real solution is to parse the expressions and create something like an abstract syntax tree. You also need to analyze the tree and prune any tautological branches like || false and && true which are trivial and add no semantic meaning to the expression. Then you need to compare the pruned trees for each existing expression to the tree for the new expression, allowing for things like A || B and B || A to be equivalent (logical OR operator with propositions A and B, order doesn't matter because OR is commutative).

Maybe you can propose this as a final year research project in a formal verification or compilers course :-)

@andrewazores andrewazores merged commit c4b05e6 into cryostatio:main Jul 14, 2022
@andrewazores andrewazores deleted the matchexpression-credentials branch July 14, 2022 20:49
@tthvo
Copy link
Member

tthvo commented Jul 14, 2022

Yehhh that is very niceee ideaa! I have been actually thinking about this issue for a while but never gets to play around with it (too much thinking haha). I guess maybe I could do it as a side-quest every now and then too 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request needs-documentation
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Implement enhanced Credentials creation form with Match Expression Evaluator
4 participants